Conversation
ba6f6e7 to
627e97c
Compare
JohnThomson
left a comment
There was a problem hiding this comment.
This is hard to review. A lot of code moved and it's not clear whether it changed significantly or just got refactored. Also, a lot of the code feels 'magical', since I know little about service workers. Probably a lot of it was CoPilot. I've gone on the principle that since it works and seems vaguely reasonable, it's probably OK. It's all only going to affect books with this new feature, and we can quickly deploy a fix to Blorg if we need to. I picked up a couple of minor things. If there's anything you think I should particularly pay attention to, please point it out.
@JohnThomson reviewed 3 files and all commit messages, and made 3 comments.
Reviewable status: 3 of 11 files reviewed, 2 unresolved discussions (waiting on nabalone).
src/book-navigation-interceptor-sw.js line 127 at r1 (raw file):
const data = await response.json(); return data.results?.[0];
Between this and the limit: "1" above, it seems you decided to ignore the possibility that there is more than one matching book. I think that's OK...this is a big step forward even without such error handling, and hopefully people who upload linked collections of books know enough to avoid such mistakes, and even if the mistake is made the book it takes you to has some hope of being relevant. Might be worth noting, though, in a comment.
src/model/BookUrlUtils.ts line 46 at r1 (raw file):
filePath: string ): string | undefined { // TODO update comment
Looks like this still needs attention
JohnThomson
left a comment
There was a problem hiding this comment.
@JohnThomson reviewed 8 files.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on nabalone).
627e97c to
920ad19
Compare
nabalone
left a comment
There was a problem hiding this comment.
Besides the new service worker and its usage, I think the rest is just refactoring to make accessible the shared logic which the service worker now needs
@nabalone made 3 comments.
Reviewable status: 9 of 11 files reviewed, 2 unresolved discussions (waiting on JohnThomson).
src/book-navigation-interceptor-sw.js line 127 at r1 (raw file):
Previously, JohnThomson (John Thomson) wrote…
Between this and the limit: "1" above, it seems you decided to ignore the possibility that there is more than one matching book. I think that's OK...this is a big step forward even without such error handling, and hopefully people who upload linked collections of books know enough to avoid such mistakes, and even if the mistake is made the book it takes you to has some hope of being relevant. Might be worth noting, though, in a comment.
Done. Also further limited the query, hopefully that will help a little
src/model/BookUrlUtils.ts line 46 at r1 (raw file):
Previously, JohnThomson (John Thomson) wrote…
Looks like this still needs attention
Done.
This PR addresses the issue BL-15694 by adding support for navigation links on blorg.
This change is